- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6
Implement merge strategy based on TYPE+TARGET #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
89f9260    to
    46d7006      
    Compare
  
    46d7006    to
    28dc04d      
    Compare
  
    28dc04d    to
    74512d8      
    Compare
  
    | 
 | 
74512d8    to
    ad6bff9      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In summary:
- I would make the MergeStrategyinterface public and simplify it as suggested.
- Make everything needed by merge strategies available as public symbols.
- I would make merge strategies take any state they need via the constructor, not the filter function.
- Expose the cached dispatches via the Dispatcherdirectly.
ad6bff9    to
    c6ddf66      
    Compare
  
    | Updated @llucax | 
| I solved avoiding passing the service now by just using the type instead | 
c6ddf66    to
    f774825      
    Compare
  
            
          
                src/frequenz/dispatch/_bg_service.py
              
                Outdated
          
        
      |  | ||
| @property | ||
| @abstractmethod | ||
| def filter(self) -> Callable[[Dispatch], bool]: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this still returning a callable and is not the callable itself? 🤔
        
          
                src/frequenz/dispatch/_bg_service.py
              
                Outdated
          
        
      |  | ||
| async def new_running_state_event_receiver( | ||
| self, type: str, *, unify_running_intervals: bool = True | ||
| self, type: str, *, merge_strategy: type[MergeStrategy] | None = None | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still prefer the approach of instantiating the merge strategy from outside and passing the instance for a couple of reasons:
- Flexibility. A merge instance could potentially need to keep more state than just the scheduler, if we instantiate it inside the function we remove that flexibility of allowing merge strategies have the state they want.
- Reuse: We could potentially reuse the same merge strategy instance with many different actors (this is minor, the above is a stronger point, but still).
If the idea is to make the typical use easier, I liked the idea of adding a wrapper for common merge strategies in Dispatcher for example, like Dispatcher.mergeByWhatever(), but I think the design/architecture of merge strategies in general should be flexible.
Then I also don't think it is much worse from the user perspective to use MergeByWhatever(dispatcher) than dispatcher.mergeByWhatever(), and it might be generally useful to have dispatcher.dispatches exposed publicly as read-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the idea is to make the typical use easier, I liked the idea of adding a wrapper for common merge strategies in Dispatcher for example, like Dispatcher.mergeByWhatever(), but I think the design/architecture of merge strategies in general should be flexible.
I thought about this approach, but at the end it wouldn't save anything because then you have to type
- dispatcher.new_running_state_event_receiver("TEST", dispatcher.mergeByExample)instead of
- dispatcher.new_running_state_event_receiver("TEST", MergeByExample(dispatcher))
So that seemed a bit useless.
Only a full wrapper like
- dispatcher.new_rst_event_receiver_merged_by_example("TEST")
would make some usage difference..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. So combined to what we discussed in #104 I guess we agree that the end goal would be to end up with: dispatcher.manage(type="FCR", actor_factory=.., strategy=MergeByType(dispatcher)).
On the other hand... If we are merging, I guess you always need to have the full list of dispatches, otherwise how would you merge? People can still create merge strategies that require more state, but I guess the list of available dispatches is the minimum you need to do any merging on the typical case.
What about?
class MergeStrategy(ABC):
    @abstractmethod
    def filter(self, all_dispatches: Map[int, Dispatch], current_dispatch: Dispatch) -> bool: ...
...
        if merge_strategy:
            receiver = receiver.filter(functools.partial(merge_strategy.filter, self._dispatches))
            # or lambda d: merge_strategy.filter(self._dispatches, d)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. It doesn't have the unified method manage yet, I intend to add that in a later PR
de35812    to
    38c504a      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments, but looking preeeeety goooood!
Implement merge strategy based on TYPE+TARGET Signed-off-by: Mathias L. Baumann <[email protected]>
38c504a    to
    61043e5      
    Compare
  
    Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
61043e5    to
    93683e2      
    Compare
  
    | Updated, addressed all comments | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! 🎉
No description provided.